-
Notifications
You must be signed in to change notification settings - Fork 15k
[Flang-RT][libc] LLVM-independent unittests #164794
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| CFI_attribute_t attribute, CFI_type_t type, std::size_t elem_len, | ||
| CFI_rank_t rank, const CFI_index_t extents[]) { | ||
| #ifdef VERBOSE | ||
| DumpTestWorld(base_addr, attribute, type, elem_len, rank, extent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not even compile (extents)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit under the weather at the moment so I have only read this briefly. My takeaways:
- The impact to the flang-rt tests is minimal.
- If someone did use llvm things in those tests, it would fail at link time (perhaps earlier if the include paths are also not added anymore).
- Doing this for just flang-rt right now is in theory fine, because compiler-rt has done the same thing in its own way.
- There's an impact on cmake used by other projects though, so we need some review from them.
For instance, Clang is built using gcc which uses libstdc++, but the runtimes is built by Clang which can be configured to use libcxx by default.
In our buildbot we attempt to link everything to the libc++ from the installed clang, so in theory we can pass enough flags to fix the issue in other ways (though I never did find the right runes to do it).
Your example though, I see why that cannot be done. Even if we added test specific library choices, those tests would link to a runtime library linked against something else.
Saying that you can't run tests in this scenario, of course we want to avoid that.
I will try this patch with our buildbot config, but expect it to be about a week before I get time to do so.
| if (NOT TARGET llvm_gtest) | ||
| if (NOT TARGET default_gtest) | ||
| message(WARNING "Flang-RT unittests disabled due to GTest being unavailable; " | ||
| "Try LLVM_INSTALL_GTEST=ON for the LLVM build") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that this warning will change or become unreachable.
The LLVM-customized GTest has a dependency on LLVM to support
llvm::raw_ostreamand hence has to link to LLVMSupport. The runtimes use the LLVMSupport from the bootstrapping LLVM build. The problem is that the boostrapping compiler and the runtimes target can diverge in their ABI, even in the runtimes default build. For instance, Clang is built using gcc which uses libstdc++, but the runtimes is built by Clang which can be configured to use libcxx by default. This has caused flang-aarch64-libcxx) to break, and is still broken.This patch makes the runtimes' GTest independent from LLVMSupport so we do not link any runtimes component with LLVM components.
Runtime projects that use GTest unittests:
The current state of this PR tries to reuse https://github.com/llvm/llvm-project/blob/main/third-party/unittest/CMakeLists.txt as much as possible, altough personally I would prefer to make it use "modern CMake" style. A new macro
build_gtestadds a configurable build of GTest, one for LLVM (llvm_gtest, NFCI) and another one for the runtimes (runtimes_gtest). It is not possible to reusellvm_gtestfor both sincellvm_gtestis imported usingfind_package(LLVM)if LLVM_INSTALL_GTEST. An aliasdefault_gtestis used to select between the two.default_gtestcould also be used for openmp which also supports standalone and LLVM_ENABLE_PROJECTS build mode.